Add: gowin nextpnr build #10
Conversation
|
Omg, I forgot the +x on the test script on smoke-tests. Fixing it hdl/smoke-tests#2 |
racerxdl
force-pushed
the
main
branch
2 times, most recently
from
39b7e98
to
14d8e66
Compare
There was a problem hiding this comment.
First off, thanks for contributing! It looks good for a first approach, but we need to clarify a few things. Please see the comments below.
Do not worry about test 'impl' failing for now. If you check the graph (https://hdl.github.io/containers/#_graph) it depends on image nextpnr, so it will keep failing until this PR is done and merged. That's ok.
.github/workflows/nextpnr.yml
Outdated
| @@ -46,12 +46,14 @@ jobs: | |||
| - run: dockerBuild nextpnr:ice40 nextpnr ice40 | |||
| - run: dockerBuild nextpnr:icestorm nextpnr icestorm | |||
| - run: dockerBuild nextpnr:ecp5 nextpnr ecp5 | |||
| - run: dockerBuild nextpnr:gowin nextpnr gowin | |||
There was a problem hiding this comment.
Please, place gowin below prjtrellis. Those are couples:
- ice40 and icestorm
- ecp5 and prjtrellis
- gowin and apicula
nextpnr.dockerfile
Outdated
| FROM build AS build-gowin | ||
| RUN mkdir -p /tmp/nextpnr/build \ | ||
| && apt-get update -qq \ | ||
| && DEBIAN_FRONTEND=noninteractive apt-get -y install --no-install-recommends python3-setuptools python3-pip \ |
There was a problem hiding this comment.
I need some background about apicula/apycula. When building nextpnr-ice40 and nextpnr-ecp5, some assets are required, but not executables/scripts. If that is the case with nextpnr-gowin, then you should get the assets here instead of installing the Python package.
Apart from that, icestorm and prjtrellis are built in separated dockerfiles/workflows, from sources. Overall, each tool is built/installed once only. We should do the same for apicula/apycula. Then, this stage would COPY from hdlc/pkg:apicula.
There was a problem hiding this comment.
Actually is a python library which also provides a link for running it as a executable. The nextpnr-gowin needs the library itself so it would just save some symbolic links.
I will make the adjusts to be at pkg and then just copy :D
There was a problem hiding this comment.
Actually is a python library which also provides a link for running it as a executable. The nextpnr-gowin needs the library itself so it would just save some symbolic links.
Does nextpnr-gowin actually call apicula/apycula Python scripts at runtime? I would expect it to be required when building nextpnr-gowin, but then it should be independent (as nextpnr-ice40 or nextpnr-ecp5).
I will make the adjusts to be at pkg and then just copy :D
Thanks! You can take Symbiyosys' dockerfile as an example. However, that one has an explicit make install, which makes it easier to handle regardless of PIP. I had a quick look at apicula and I didn't find a similar feature.
Note that, when creating the package, the PREFIX of the tools needs to be /usr/local; and DESTDIR should be used for placing them in a temporary location (to be then copied to the package image). I don't know whether the PREFIX is relevant for apicula.
There was a problem hiding this comment.
Just checked, after building it it doesnt need it. So I removed from the nextpnr container. Apicula is python so there is no make install, just pip and easy_install which doesnt support prefix, only target which does not generate the "binaries".
|
|
||
| #--- | ||
|
|
||
| FROM base AS gowin |
There was a problem hiding this comment.
There should be two stages here:
FROM base AS gowin
and
FROM gowin AS apicula
COPY --from=hdlc/pkg:apicula /apicula /
The first one would contain artifacts of nextpnr-gowin only; and the second one would include apicula/apycula too.
| @@ -110,3 +138,6 @@ RUN cd /tmp/nextpnr/build \ | |||
|
|
|||
| FROM base AS all | |||
There was a problem hiding this comment.
This last "all" stage should include all nextpnr-* artifacts: nextpnr-ice40, nextpnr-ecp5, nextpnr-gowin and nextpnr-generic. Maybe it would be desirable to change this to build generic only, since others are built before:
#---
FROM build AS build-all
RUN cd /tmp/nextpnr/build \
&& cmake .. \
-DARCH=generic \
-DBUILD_GUI=OFF \
-DBUILD_PYTHON=ON \
-DUSE_OPENMP=ON \
&& make -j $(nproc) \
&& make DESTDIR=/opt/nextpnr install
#---
FROM base AS all
COPY --from=build-ice40 /opt/nextpnr /
COPY --from=build-ecp5 /opt/nextpnr /
COPY --from=build-gowin /opt/nextpnr /
COPY --from=build-generic /opt/nextpnr /Note that this last image does not include icestorm, prjtrellis or apicula/apycula. We can later add another image for that, on top of this stage. But I think it's better to handle that after this PR is done.
|
hmm, maybe since it will need hdlc/apicula, I should first make a PR adding apicula as a base image? |
That makes sense. hdlc/pkg:apicula and hdlc/apicula might be used regardless of nextpnr. Therefore, it will be cleaner to handle that first. BTW, @se-bi is modifying
|
umarcor
added
enhancement
This adds support for gowin nextpnr. I tested successfully with https://github.com/racerxdl/tangnano-yosis-hello locally, just had to change the docker images for my local ones.
Smoke tests on hdl/smoke-tests#1, will update soon after merge and change this from draft to pull request